Skip to content

Ensure color overlay visibility control is uniquely determined by show_color_overlay (property reflecting ToggleButton state)#51

Merged
zoccoler merged 11 commits intomainfrom
fix_overlay_bug2
May 6, 2025
Merged

Ensure color overlay visibility control is uniquely determined by show_color_overlay (property reflecting ToggleButton state)#51
zoccoler merged 11 commits intomainfrom
fix_overlay_bug2

Conversation

@zoccoler
Copy link
Contributor

@zoccoler zoccoler commented May 5, 2025

This pull request includes several updates to improve code clarity, maintainability, and functionality in the biaplotter library. The most important changes are improving the behavior of artist visibility and overlay handling.

General Updates:

  • Updated the library version from 0.3.0 to 0.3.1 in src/biaplotter/__init__.py.

Documentation Improvements:

  • Clarified the docstring for the data setter in src/biaplotter/artists_base.py to reflect that it applies to all artists, not just scatter plots.
  • Updated the docstring for the visible setter in src/biaplotter/artists_base.py to generalize its scope to all artists.
  • Reformatted the docstring for the deprecated hide_color_overlay method in src/biaplotter/plotter.py for consistency.

Functional Improvements:

  • Enhanced _set_active_artist in src/biaplotter/plotter.py to conditionally show the overlay of the active artist only when show_color_overlay is enabled.

Code Cleanup:

  • Simplified a signal connection in _connect_signals by collapsing a multi-line statement into a single line in src/biaplotter/plotter.py.

zoccoler added 8 commits May 5, 2025 13:37
…ting normalization for categorical colormap and non-linear norm method (this avoids calling _colorize again which ends up calling _get_normalization a second time)

This avoids a bug that creates an extra overlay image (matplotlib artist) that is not included in the _mpl_artists dictionary. The practical effect of this is having a fixed, always shown, histogram color overlay.
…en updating other properties, update the internal variables, but don't display them until `overlay_visible` is `True`

Add `self._scatter_overlay_rgba` to `Scatter` and `self._overlay_histogram_rgba` to `Histogram2D`. Add `_update_overlay_visibility` helper method to both classes.
…olor_indices` would be `None` and/or `self._scatter_overlay_rgba` would be `None`)
@zoccoler zoccoler requested a review from Copilot May 5, 2025 20:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the overlay handling logic for active artists and refines documentation and code clarity across the biaplotter library. Key changes include:

  • Simplified signal connection in _connect_signals.
  • Updated docstrings in artists_base.py to generalize their scope.
  • Enhanced _set_active_artist to conditionally show overlays based on show_color_overlay, and updated the library version.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/biaplotter/plotter.py Simplified signal connection, refined overlay logic in _set_active_artist, and updated deprecated method formatting.
src/biaplotter/artists_base.py Generalized docstrings for data and visible setters.
src/biaplotter/init.py Updated the library version to 0.3.1.

@codecov
Copy link

codecov bot commented May 5, 2025

Codecov Report

Attention: Patch coverage is 90.59829% with 11 lines in your changes missing coverage. Please review.

Project coverage is 90.83%. Comparing base (530492b) to head (2eca8c8).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/biaplotter/artists.py 90.26% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   90.47%   90.83%   +0.36%     
==========================================
  Files           9        9              
  Lines         976      982       +6     
==========================================
+ Hits          883      892       +9     
+ Misses         93       90       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jo-mueller
Copy link
Contributor

@zoccoler Checking the functionality on the clusters-plotter end again with these changes and thinking about it again, I'd be fine with waving it through. If some things can be refactored, this can be done later, too. I'll keep the refactoring to the clusters-plotter and will just wave this through, if you don't mind?

@zoccoler
Copy link
Contributor Author

zoccoler commented May 6, 2025

@zoccoler Checking the functionality on the clusters-plotter end again with these changes and thinking about it again, I'd be fine with waving it through. If some things can be refactored, this can be done later, too. I'll keep the refactoring to the clusters-plotter and will just wave this through, if you don't mind?

No problem. In any case, I think #53 solves it without breaking it as before. We were close.
If you can test that branch and it works, please merge there and we finally merge here. If it fails on clusters-plotter side, we close that and merge only here.

Move overlay_visible functionality to _colorize
@zoccoler zoccoler changed the title Ensure color overlay control is uniquely determined by show_color_overlay (property reflecting ToggleButton state) Ensure color overlay visibility control is uniquely determined by show_color_overlay (property reflecting ToggleButton state) May 6, 2025
@zoccoler zoccoler merged commit ac8ffc0 into main May 6, 2025
21 checks passed
@jo-mueller jo-mueller deleted the fix_overlay_bug2 branch July 3, 2025 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants